Allow BooleanField(null=True) on Django >= 2.1 #38
Conversation
as this now recommened instead of using NullBooleanField.
|
Changed in Django 2.1: In older versions, this field doesn’t permit null=True, so you have to use NullBooleanField instead. Using the latter is now discouraged as it’s likely to be deprecated in a future version of Django. |
|
@rocioar My PR fails because django is not importable in tests. Do you have suggestion how to fix it? I would expect django to be always installed when using flake8-django. |
so tests importing Django do not fail.
Codecov Report
@@ Coverage Diff @@
## master #38 +/- ##
========================================
- Coverage 100% 99.5% -0.5%
========================================
Files 17 11 -6
Lines 270 203 -67
========================================
- Hits 270 202 -68
- Misses 0 1 +1
Continue to review full report at Codecov.
|
as recommended in pytest-cov doc.
|
@rocioar I'm trying to fix it by running the coverage report from I'm not sure why it fails now but can you please review my PR and say if you are OK with this direction? |
| ] | ||
| NOT_BLANK_TRUE_FIELDS = ['BooleanField'] | ||
| NOT_BLANK_TRUE_FIELDS = [] |
There was a problem hiding this comment.
The tests are failing now, because coverage decreases from 100% to 99% due to tests on test_model_fields where NOT_BLANK_TRUE_FIELDS is uses as a parameter. Since the list is empty this tests are not running, so that part of the code is not being covered.
I think, let's remove NOT_BLANK_TRUE_FIELDS altogether, since we could it's ok to have BooleanField with blank=True even in older version, that's just going to make the system use the default value.
What are your thoughts?
| NOT_BLANK_TRUE_FIELDS = ['BooleanField'] | ||
| NOT_BLANK_TRUE_FIELDS = [] | ||
|
|
||
| if not ALLOW_NULL_BOOLEAN: |
There was a problem hiding this comment.
Could we do the check for django.VERSION here? Just so that we make it more explicit?
| skip_install = true | ||
| deps = coverage | ||
| commands = | ||
| coverage report --fail-under=100 |
There was a problem hiding this comment.
This report part is not needed, since coverage reports are being uploaded to codecov.
| [testenv:clean] | ||
| deps = coverage | ||
| skip_install = true | ||
| commands = coverage erase |
| django22-py{37,36} | ||
| django21-py{37,36,35} | ||
| django20-py{36,35,34} | ||
| report |
There was a problem hiding this comment.
Let's remove clean and report from the envlist. We don't want to run coverage for every environment.
| command: | | ||
| . venv/bin/activate | ||
| pip install -e . | ||
| py.test --cov=. --cov-fail-under=100 |
There was a problem hiding this comment.
Instead of removing this code, let's create a requirements/test.txt and add Django as a requirements for tests.
|
Actually, I spent more time thinking about this, and I think we should actually drop support for Django < 2.1, since Django is no longer supporting those versions. That being said, we can remove this check altogether. |
|
@rocioar Yes, removing this check makes sense to me. |
|
I've removed the check on: #60 |
as this now recommened instead of using NullBooleanField.